Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Edit scheduleEntry on activity view without redirection problem #4975

Open
wants to merge 18 commits into
base: devel
Choose a base branch
from
Open

Edit scheduleEntry on activity view without redirection problem #4975

wants to merge 18 commits into from

Conversation

manuelmeister
Copy link
Member

@manuelmeister manuelmeister commented Apr 14, 2024 •
edited
Loading

Now we have a better fallback for wrong/missing ids for activities and scheduleEntries. If a scheduleEntry is not found, but the activity is found, we load the first occurring scheduleEntry of the activity. If for whatever reason we can't find the activityId, then we try to find it using the scheduleEntryId.

This now also works if we navigate from scheduleEntry A to scheduleEntry B, then delete scheduleEntry A and use the browser back. Before this would just display our 404.

Replaces #4500

@manuelmeister manuelmeister marked this pull request as draft April 14, 2024 11:01
@manuelmeister manuelmeister added the WIP Work in progress label Apr 14, 2024
@manuelmeister manuelmeister mentioned this pull request Apr 14, 2024
Closed
@BacLuc BacLuc mentioned this pull request Apr 27, 2024
Open
2 tasks
@manuelmeister manuelmeister force-pushed the feature/schedule-entry-editing-in-activity branch from 4ad1317 to 3fe670b Compare June 9, 2024 12:25
@manuelmeister manuelmeister added this to the Improvements (v3.2) milestone Dec 3, 2024
@manuelmeister manuelmeister changed the title WIP: Add schedule entry edit Find a solution for the deletion redirect problem Dec 3, 2024
@manuelmeister manuelmeister removed the WIP Work in progress label Dec 3, 2024
@manuelmeister manuelmeister self-assigned this Dec 3, 2024
@manuelmeister manuelmeister force-pushed the feature/schedule-entry-editing-in-activity branch 4 times, most recently from 2569d56 to 88040c3 Compare December 25, 2024 19:16
@manuelmeister manuelmeister force-pushed the feature/schedule-entry-editing-in-activity branch from 88040c3 to f1a84b6 Compare February 8, 2025 17:47
@manuelmeister manuelmeister added the deploy! Creates a feature branch deployment for this PR label Feb 8, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 8, 2025 •
edited
Loading

✅ Feature branch deployment ready!

Name Link
😎 Deployment https://pr4975.ecamp3.ch/
🔑 Login [email protected] / test
🕒 Last deployed at Sun Feb 23 2025 12:01:15 GMT+0100
🔨 Latest commit 203b7f40f3a78778e21c1dc3b2b2b20d497f9ee1
🔍 Latest deploy log https://github.com/ecamp/ecamp3/actions/runs/13482340920/job/37669182414
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

@manuelmeister manuelmeister changed the title Find a solution for the deletion redirect problem Edit scheduleEntry on activity view without redirection problem Feb 8, 2025
@manuelmeister manuelmeister requested a review from a team February 8, 2025 19:31
BacLuc
BacLuc reviewed Feb 9, 2025
View reviewed changes
Copy link
Contributor

@BacLuc BacLuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good.
Is robust, even switches the day when you switch the day of the currently displayed scheduleentry.

but if someone else deletes the scheduleEntry you currently display, and then you press "Submit" (or aktualisieren), then it ends in an infinite loop. But it works if you navigate to a scheduleEntry which was deleted.
maybe you need one + renavigation more.

@manuelmeister
Copy link
Member Author

but if someone else deletes the scheduleEntry you currently display, and then you press "Submit" (or aktualisieren), then it ends in an infinite loop. But it works if you navigate to a scheduleEntry which was deleted.

maybe you need one + renavigation more.

I don't understand. Wasn't this a problem before? For that we would probably need a callback on the entity, that triggers on deletion.

@BacLuc
Copy link
Contributor

BacLuc commented Feb 9, 2025

but if someone else deletes the scheduleEntry you currently display, and then you press "Submit" (or aktualisieren), then it ends in an infinite loop. But it works if you navigate to a scheduleEntry which was deleted.
maybe you need one + renavigation more.

I don't understand. Wasn't this a problem before? For that we would probably need a callback on the entity, that triggers on deletion.

No before, this lead to a 404. (which is correct i think)
It is a weird edgecase.
No: you yourself cannot delete the scheduleentry you are displaying not in this tab.
But you can do it in another tab (with a completely different javascript runtime), so callbacks on delete don't help.

@manuelmeister
Copy link
Member Author

@BacLuc so how would you fix this? Do you think this is a big issue?

@BacLuc
Copy link
Contributor

BacLuc commented Feb 9, 2025

You could catch the 404 for the not existing scheduleentry and change the route to an error page.
I don't know if we need to fix this...its for the team to decide.

@BacLuc
Copy link
Contributor

BacLuc commented Feb 10, 2025

Here is the video for you

Screencast.from.2025-02-10.23-39-19.webm

image

@manuelmeister
Copy link
Member Author

manuelmeister commented Feb 12, 2025 •
edited
Loading

@BacLuc it works now. I just check each scheduleEntry patch. If the request returns 404 and it is the current one, we redirect to an existing one.

@manuelmeister manuelmeister requested a review from a team February 14, 2025 17:51
BacLuc
BacLuc approved these changes Feb 21, 2025
View reviewed changes
@simfeld simfeld added deploy! Creates a feature branch deployment for this PR and removed deploy! Creates a feature branch deployment for this PR labels Feb 21, 2025
simfeld
simfeld reviewed Feb 22, 2025
View reviewed changes
Copy link
Contributor

@simfeld simfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand it fully but something doesn't quite add up in the "browser back when previous schedule entry deleted" case: it finds another scheduleEntry of the activity, but some data is not refreshed properly. See screenshot where the title and time slot matches with a green LS entry but the badge says blue LP.

image

@manuelmeister manuelmeister deployed to feature-branch February 23, 2025 11:00 — with GitHub Actions Active
@manuelmeister manuelmeister requested review from simfeld and a team February 23, 2025 11:05
@manuelmeister
Copy link
Member Author

@simfeld I've fixed the problem. Also some other edge cases where we don't execute requireActivityScheduleEntry (browser-back, activity switch, scheduleEntry switch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlobeltrame carlobeltrame carlobeltrame left review comments

@BacLuc BacLuc BacLuc approved these changes

@simfeld simfeld Awaiting requested review from simfeld

At least 2 approving reviews are required to merge this pull request.

Assignees

@manuelmeister manuelmeister

Labels
deploy! Creates a feature branch deployment for this PR
Projects
None yet
Milestone
Updates and improvements (v3.2)
Development

Successfully merging this pull request may close these issues.
4 participants
@manuelmeister @BacLuc @carlobeltrame @simfeld